feat(datasets): add PaginatedAPIDataset - #1482
Conversation
|
Thank you for your interest in contributing to Kedro! We appreciate the effort, but we can only accept contributions for issues listed on our open source ticket board: OS contribution tasks. Other issues in the repository are not yet ready for external contribution — they may need further scoping or are marked for internal work. If you'd like to work on something that isn't on the board, please reach out to us via Slack (https://slack.kedro.org/) or open a GitHub issue to discuss it first. This helps us align on the approach and gives your contribution the best chance of being accepted. For more details, please see our contribution guidelines: https://github.com/kedro-org/kedro/blob/main/CONTRIBUTING.md and the developer guide: https://github.com/kedro-org/kedro/wiki/Guidelines-for-contributing-developers. We're closing this PR for now, but we'd love to see you contribute to one of the issues on the board! |
|
Hi @ElenaKhaustova, I think I found the issue — it was my mistake. I forgot to add the issue to the PR description. This PR is based on issue #1023, which I also picked from the project board. Sorry about the confusion! Could you please open both PR's (#1481 and #1482) for review when you have a chance? |
|
Thanks for referencing the issue @Shizoqua, re-opening this for review. |
|
Thanks @Shizoqua the pagination loop looks good can you update RELEASE.md Also flagging that a new non-experimental dataset needs TSC sign-off we'll discuss whether this should start in |
|
Thanks for the feedback guys @SajidAlamQB @jitu5 . I’ve updated the example to use |
f412067 to
e441eda
Compare
|
Hi @Shizoqua, Thanks for the contribution. Since this adds a new non-experimental dataset, it will need sign off from at least half of the TSC before we can merge. A few things to address before merge: Critical kedro-datasets/RELEASE.md: the entry was added under # Release 9.6.0 instead of # Upcoming release. Could you move it to the upcoming section at the top? Suggestions
Overall the implementation looks solid and the earlier review feedback on credentials, host trust, and release notes seems addressed. Once the RELEASE placement is fixed and we have TSC alignment on dataset placement, this should be in good shape. Thanks again! |
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
…edAPIDataset Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
540ec6d to
b3b099e
Compare
|
Hi @ravi-kumar-pilla Thanks for the review! Pushed fixes:
Let me know if anything else needs adjusting |
Description
APIDatasetdoes not provide a way to retrieve and combine data spread across multiple API pages. This PR adds a maintained JSON pagination dataset for APIs that return a next-page URL and a list of results.fixes #1023
Development notes
PaginatedAPIDataset, exported fromkedro_datasets.api.next_url_pathandresults_pathconfiguration.max_pages.Developer Certificate of Origin
Signed off
Checklist
jsonschema/kedro-catalog-X.XX.jsonif necessaryRELEASE.mdfile